Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bitops #69

Merged
merged 2 commits into from
Mar 20, 2024
Merged

Add bitops #69

merged 2 commits into from
Mar 20, 2024

Conversation

rolandlo
Copy link
Collaborator

luaffi-tkl, which is used on Lua 5.x to replace the LuaJIT builtin ffi module, has dropped shipping its bit-module (which only worked on Lua >= 5.3). This PR replaces the bit-module via bitops.lua. Only the bitwise and on two arguments is needed.

For LuaJIT: use builtin bit.band
For Lua 5.1, 5.2: use bit.band from luabitop if available
For Lua 5.3, 5.4: use builtin & operator

On Lua 5.1, 5.2, which are newly supported, luabitop becomes a dependency.

Currently only bit.band is needed.

For LuaJIT: use builtin bit.band
For Lua 5.1, 5.2: use bit.band from luabitop if available
For Lua 5.3, 5.4: use builtin & operator
@rolandlo rolandlo merged commit 7101e1b into libvips:master Mar 20, 2024
6 checks passed
@rolandlo rolandlo deleted the add-bitops branch March 20, 2024 14:40
@RiskoZoSlovenska
Copy link
Contributor

RiskoZoSlovenska commented Mar 20, 2024

Wouldn't it be simpler and easier to just specify luabitop as a LuaRocks dependency and use that on all versions (including LuaJIT)?

Personally, I'd like to try and get rid of the Makefile in the future; LuaRocks should support doing almost everything the Makefile does.

@rolandlo
Copy link
Collaborator Author

Wouldn't it be simpler and easier to just specify luabitop as a LuaRocks dependency and use that on all versions (including LuaJIT)?

luabitop requires lua >= 5.1, < 5.3. If we make luabitop as a dependency of lua-vips we can't install lua-vips for versions 5.3 and 5.4 any more, I fear. Or is there a way to specify dependencies only for particular Lua versions?

@RiskoZoSlovenska
Copy link
Contributor

Oh, hmm, that's annoying. I'm not aware of any way to specify dependencies based on version, unfortunately.

@RiskoZoSlovenska
Copy link
Contributor

Perhaps we could use https://luarocks.org/modules/siffiejoe/bit32 instead?

@rolandlo
Copy link
Collaborator Author

Perhaps we could use https://luarocks.org/modules/siffiejoe/bit32 instead?

It looks like that would work. For LuaJIT it's much slower (by a factor of roughly 40 in my tests) though than the builtin bit.band and it's also slower (but only by about a factor of 2) than the & operator from Lua 5.3/5.4. So I wouldn't want to use band from bit32 for LuaJIT and Lua 5.3/5.4.

@RiskoZoSlovenska
Copy link
Contributor

Did you benchmark band by itself or in the context of voperation.set and voperation.call (the places where it's used)? As in, is the performance when calling operations actually substantially affected?

@rolandlo
Copy link
Collaborator Author

I benchmarked band by itself. I don't know how much it affects the performance of voperation.set and voperation.call.

@RiskoZoSlovenska
Copy link
Contributor

I'd benchmark the latter before eliminating bit32 as underperformant; in the grand scheme of things, it's the voperation methods that we care about, and bit32's raw performance doesn't matter if it doesn't actually slow them down much.

@rolandlo
Copy link
Collaborator Author

I'd benchmark the latter before eliminating bit32 as underperformant; in the grand scheme of things, it's the voperation methods that we care about, and bit32's raw performance doesn't matter if it doesn't actually slow them down much.

In case the band performance doesn't matter much we could also consider using a hardcoded fallback that is used when neither the bit nor the bit32 modules are installed. Then there wouldn't be any need for a dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants